-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(js): free native strings #238
Conversation
Signed-off-by: Ariel Gentile <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't include the RN implementation yet right?
@@ -723,6 +726,8 @@ export class NodeJSAnoncreds implements Anoncreds { | |||
const returnValue = handleReturnPointer<{ data: Buffer; len: number }>(ret) | |||
const output = new Uint8Array(byteBufferToBuffer(returnValue)) | |||
|
|||
this.nativeAnoncreds.anoncreds_buffer_free(returnValue.data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a buffer also copied and not a ref?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just done an update there because when creating an Uint8Array
from a Buffer
(this case in line 727) simply a new view is created, meaning (if I understand correctly) that no data is copied.
So I think it is safer to do this buffer free right after doing the text decoding and outputting to a string (which effectively copies the data).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would depend on how ffi-napi
ref-napi
handles this. It seems to me that they copy the data, but I haven't checked the code, yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. TypeScript 5.2.2
has been released so we can use the new using
declaration to probably make it even better. This could even be done in the shared
part of the library fixing it for Node.JS and React Native
Signed-off-by: Ariel Gentile <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
I couldn't test it deeply but I've updated those template |
Absolutely! I was looking at the release notes and |
Not completely sure if it's the right approach (especially in react-native) but here is a simple attempt to free strings automatically in JS wrappers before returning to the caller, as discussed in #236.